Skip to content

fix: skip heartbeat check for main dataset if no LLM channel - #5139

Open
WeijuanShao wants to merge 5 commits into
webex:nextfrom
WeijuanShao:fix/hashtree-watchdog-issue
Open

fix: skip heartbeat check for main dataset if no LLM channel#5139
WeijuanShao wants to merge 5 commits into
webex:nextfrom
WeijuanShao:fix/hashtree-watchdog-issue

Conversation

@WeijuanShao

@WeijuanShao WeijuanShao commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

COMPLETES #https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-841885

This pull request addresses

skip heartbeat check for main dataset if no LLM channel

by making the following changes

Added a meeting-scoped LLM expected callback into HashTreeParser callbacks.
Updated heartbeat watchdog logic so main dataset skips timer setup only when:
the callback is provided, and the callback explicitly reports that current-meeting LLM is not expected.

Change Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Tooling change
  • Internal code refactor

The following scenarios were tested

< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >

The GAI Coding Policy And Copyright Annotation Best Practices

  • GAI was not used (or, no additional notation is required)
  • Code was generated entirely by GAI
  • GAI was used to create a draft that was subsequently customized or modified
  • Coder created a draft manually that was non-substantively modified by GAI (e.g., refactoring was performed by GAI on manually written code)
  • Tool used for AI assistance (GitHub Copilot / Other - specify)
    • Github Copilot
    • Other - Please Specify
  • This PR is related to
    • Feature
    • Defect fix
    • Tech Debt
    • Automation

I certified that

  • I have read and followed contributing guidelines
  • I discussed changes with code owners prior to submitting this pull request
  • I have not skipped any automated checks
  • All existing and new tests passed
  • I have updated the documentation accordingly

Make sure to have followed the contributing guidelines before submitting.

@WeijuanShao
WeijuanShao requested review from a team as code owners August 3, 2026 08:12
Comment thread packages/@webex/plugin-meetings/src/locus-info/index.ts Outdated

if (
ownership?.currentOwner &&
ownership.currentOwner !== this.meetingId &&

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure we need this check to have all these conditions? isn't checking isOwner enough? the checks here look the same like what is already done inside resolveSessionOwnership()

Comment thread packages/@webex/plugin-meetings/src/locus-info/index.ts Outdated
}

if (
dataSet.name === DataSetNames.MAIN &&

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure it's only MAIN dataset? what about others datasets that are sent over LLM like atd-unmuted, atd-active?
We already have LLM_DATASET_NAMES defined that contains them, maybe it should be used here?

);

// eslint-disable-next-line no-continue
continue;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code here handles the case when we call resetHeartbeatWatchdogs() for main and LLM is not connected, but what about the case when LLM is not connected yet, but it is connecting and later when it becomes connected, we will end up with no heartbeat watchdog. I think we need to check if we need to restart any heartbeat watchdogs when LLM changes from "not connected" to "connected".

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8904a430dd

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

}

if (
dataSet.name === DataSetNames.MAIN &&

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Cover all LLM-backed watchdogs

When LLM is disconnected in a webinar/panelist meeting where atd-active or atd-unmuted is visible, this DataSetNames.MAIN guard leaves their watchdogs armed even though those datasets are also LLM-delivered (LLM_DATASET_NAMES includes them). Their timers will still expire and re-enqueue /sync repeatedly while the channel is down, so the no-LLM suppression only fixes main; use the LLM dataset set here instead of the single dataset check.

Useful? React with 👍 / 👎.

isLlmConnected: () => {
const llm = this.webex?.internal?.llm;

if (!llm?.isConnected?.()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed 1-1 on webex, we'll need to change this to check if we expect LLM to be connected instead of checking if it is actually connected

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c2184fee31

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

callbacks: {
locusInfoUpdateCallback: this.updateFromHashTree.bind(this, locusUrl),
syncLatencyTracker: this.callbacks.syncLatencyTracker,
isLlmExpected: () => this.parsedLocus.self?.joinedWith?.state === 'JOINED',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use actual LLM channel state

When the LLM websocket drops while the meeting is still joined (for example, connect failed or the socket is reconnecting), joinedWith.state remains JOINED, so this callback reports true and the new watchdog guard still arms the main timer and keeps issuing /sync even though no LLM messages can arrive. Meeting#updateLLMConnection already treats joined state and webex.internal.llm.isConnected() as separate values, so pass the actual connection/ownership state here, or combine it with joined state, instead of using joinedWith alone.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2c74f5a5d6

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +1841 to +1845
if (
LLM_DATASET_NAMES.includes(dataSet.name) &&
this.callbacks.isLlmExpected &&
!this.callbacks.isLlmExpected()
) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Suppress already-armed LLM watchdogs

When LLM becomes not expected after an LLM dataset watchdog has already been scheduled, no further LLM dataset message is guaranteed to arrive and re-enter this guard, so the existing timeout still fires and enqueues /sync before the guard is evaluated again. In that path performSync() also restarts the root-hash sync timer, so the no-LLM case can continue issuing syncs; the expectation check needs to run in the timeout/enqueue path or LLM timers need to be cleared when the meeting stops expecting LLM.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants